-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn user if cargo ament build is not installed #31
Warn user if cargo ament build is not installed #31
Conversation
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
=======================================
Coverage ? 60.54%
=======================================
Files ? 8
Lines ? 403
Branches ? 53
=======================================
Hits ? 244
Misses ? 149
Partials ? 10 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the --help
flag to test if the program exists is very clever.
I left a very minor recommendation for tweaking the way the information is presented, but I think the PR is good either way.
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Tweaking the way the warning is displayed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think this will really help users to navigate the unusual Rust dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luca-della-vedova thanks!
This PR returns an error if
cargo-ament-build
was not found, including a message on how to install it, it's pretty straightforward but a few notes:cargo install cargo-ament-build
as noted in the ros2-rust README, I thought about recommending pip but then it would cause a lot of trouble on 24.04 because virtual envs etc.colcon-cargo
package but I'm not 100% convinced it is a good idea to silently continue building with a build type different from what the users might expect, instead of loudly printing an error and failing.Test it!
section.Test it!
cargo ament-build
installedcargo-ament-build
You should see the (as I mentioned above slightly strange) failure:
Specifically the second warning is a bit odd but it will happen in all our error paths, so we should probably do something more than early returning on failure, but that is something out of scope for this specific PR